feat: Add Annotated Tag Push Support by fabiovincenzi · Pull Request #1139 · finos/git-proxy

fabiovincenzi · 2025-08-04T15:39:08Z

Description:
This PR introduces end-to-end support for annotated tag pushes

Split the push pipeline into separate branchPushChain and tagPushChain.
Update getChain() to select the correct chain based on action.type and presence of action.tag.
Render a dedicated “Tag Details” table when viewing a tag push instead of the commit diff.

Fixes #986

netlify · 2025-08-04T15:39:18Z

✅ Deploy Preview for endearing-brigadeiros-63f9d0 canceled.

Name	Link
🔨 Latest commit	`cf14102`
🔍 Latest deploy log	https://app.netlify.com/projects/endearing-brigadeiros-63f9d0/deploys/6a34fbbc21be1900081025c1

codecov · 2025-08-06T14:04:24Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 85.67%. Comparing base (fc23d58) to head (cf14102).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1139      +/-   ##
==========================================
+ Coverage   85.38%   85.67%   +0.28%     
==========================================
  Files          83       83              
  Lines        7878     7967      +89     
  Branches     1312     1342      +30     
==========================================
+ Hits         6727     6826      +99     
+ Misses       1123     1114       -9     
+ Partials       28       27       -1

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

jescalada

The core flow seems to work correctly! Just a few edge cases and an integration issue with the checkEmptyBranch that might be worth refactoring.

Also, we will be merging #973 soon which will likely cause a few conflicts here... Might want an extra pair of eyes to make sure everything is good to merge.

fabiovincenzi · 2025-08-22T10:46:02Z

Updated with the latest changes of main and manually tested also with gitlab

jescalada · 2025-08-27T09:49:38Z

    action = await proc.pre.parseAction(req);
+    // 2) Parse the push payload first to detect tags/branches
+    if (action.type === RequestType.PUSH) {
+      action = await proc.push.parsePush(req, action);


I'm not sure if I misunderstood the flow here, but isn't parsePush executing twice when pushing branches? Seems it executes here, and once again as part of the branchPushChain.

Regardless, it seems the push flow is working as expected - just not sure if this is running twice or if the parsePush prevents the double execution somehow.

A change above removed proc.push.parsePush from the branchPushChain, but I don't know why./ I think it should just be the first processor in both the branchPushChain and tagPushChain

@kriswest I've spent some time trying to understand/refactor this, and I think I see the reasoning: It makes sense for parsePush to be separate to the other actions, since it's the only place where we can determine what kind of Git object (tag, branch, etc.) is getting pushed. And it also makes sense to have different chains for different Git objects, since they are often processed differently (we don't check for commit content for tag pushes, etc.).

Since parsePush is the "odd one out" (just another pre-processor for pushes), we should probably specify this special case in our docs, and refactor our code accordingly (moving it into /src/proxy/processors/pre-processor).

I think the confusion comes from calling parsePush an Action/processor in the chain when it has become a pre-processor exclusively for pushes.

Wondering if there's a better solution from an architecture point of view. 🤔

Pinging @fabiovincenzi and @finos/git-proxy-maintainers for ideas

Thansk @jescalada, I see the issue and yes we need to start parsing the push to establish type, makes sense. I agree with moving it over the pre-processors folder and keeping it out of the individual chains..

THanks for looking at this @jescalada - yes I missed that it was doing the parsing that detected the push type. I agree with moving this to the pre-processors folder and keeping it out of the individual chains. The comments added help a lot.

Will we ever have to handle mixed pushes of both branches and tags... Doh, yes: https://stackoverflow.com/questions/3745135/push-git-commits-tags-simultaneously

We'll need to handle that case - we could detect and reject it for now?

jescalada

Looks good to me! I tested the tags and regular push flow and both work as expected with the new updates 👍🏼

Just a comment on the parsePush which I'm not sure if it's executing once or twice on pushes.

jescalada · 2025-08-27T10:11:34Z

As we're modifying the proxy logic, another pair of eyes here would be super helpful! 👀 @finos/git-proxy-maintainers

kriswest · 2026-06-10T11:22:42Z

Resolved comments that have been dealt with already

Co-authored-by: Kris West <kristopher.west@natwest.com> Signed-off-by: Fabio Vincenzi <93596376+fabiovincenzi@users.noreply.github.com>

…sePush

…into push-tags

…ages

jescalada · 2026-06-12T06:32:59Z

+ * @param {PushData} pushData - The push data
+ * @return {string} The reference name to display
+ */
+export const getRefToShow = (pushData: PushData): string => {


I'm getting an error here when pushing an empty tag via: git tag test-tag-1 && git push test-tag-1:

This also creates errors on the dashboard:

I tried patching this by modifying the call to trimPrefixRefsHeads to default to empty strings like such:

return trimPrefixRefsHeads(pushData.branch || '');

And now the empty tag push is visible on the dashboard, but only on the "All" tab, whereas I expected it to show on the "Pending" tab.

Notably, the reason for failure is checkUserPushPermission failing to find the email associated with the push:

If non-annotated tag support isn't included, we should perhaps check and throw a more descriptive error as the current one is misleading 👍🏼

Apparently the term is 'lightweight' tag. See https://git-scm.com/book/en/v2/Git-Basics-Tagging#:~:text=Git%20supports%20two%20types%20of,objects%20in%20the%20Git%20database for some reading on them.

It would probably be good to support these or throw a specific error message on them, but we'll need to classify them. We have an issue with them as the awful hack used to cover the fact we don't actually know the pusher (use the last committer email) fails here as theres no commit data. Hence, perhaps we should NOT support these for now and add an error message and a TODO until we fix that architectural flaw.

jescalada · 2026-06-12T07:52:55Z

    action = await proc.pre.parseAction(req);
+    // 2) Parse the push payload first to detect tags/branches
+    if (action.type === RequestType.PUSH) {
+      action = await proc.push.parsePush(req, action);


@kriswest I've spent some time trying to understand/refactor this, and I think I see the reasoning: It makes sense for parsePush to be separate to the other actions, since it's the only place where we can determine what kind of Git object (tag, branch, etc.) is getting pushed. And it also makes sense to have different chains for different Git objects, since they are often processed differently (we don't check for commit content for tag pushes, etc.).

Since parsePush is the "odd one out" (just another pre-processor for pushes), we should probably specify this special case in our docs, and refactor our code accordingly (moving it into /src/proxy/processors/pre-processor).

I think the confusion comes from calling parsePush an Action/processor in the chain when it has become a pre-processor exclusively for pushes.

Wondering if there's a better solution from an architecture point of view. 🤔

jescalada · 2026-06-12T07:53:29Z

    action = await proc.pre.parseAction(req);
+    // 2) Parse the push payload first to detect tags/branches
+    if (action.type === RequestType.PUSH) {
+      action = await proc.push.parsePush(req, action);


Pinging @fabiovincenzi and @finos/git-proxy-maintainers for ideas

jescalada · 2026-06-12T08:03:07Z

LGTM so far, the general flow is still working as expected, and the double parsePush issue is fixed 👍🏼

Just a few issues to iron out:

Empty tag pushes give a missing user error which is misleading
We should figure out whether to refactor the chains to contain parsePush again (which makes intuitive sense)
- Otherwise, we can make parsePush a pre-processor, considering it pre-processes pushes to define what type of Git object is being pushed, and consequentially which push chain must be executed (branchPushChain, tagPushChain or something else).

…ushes

…into push-tags

fabiovincenzi · 2026-06-19T08:24:48Z

All review comments have been addressed. Ready for a final look! @kriswest @jescalada

kriswest · 2026-06-19T17:31:00Z

      action.branch = parsedRefs[0].refName;
    }

-    // Use the first ref's commit range for the action id


These comments should stay...

github-actions Bot added the feature label Aug 4, 2025

fabiovincenzi linked an issue Aug 4, 2025 that may be closed by this pull request

Add support for pushing tags git push --tags G-Research-Forks/git-proxy#56

Open

jescalada reviewed Aug 7, 2025

View reviewed changes